Fixes #26970: fix bot name search on the Bots page#27365
Fixes #26970: fix bot name search on the Bots page#27365siddhiguptas wants to merge 31 commits intoopen-metadata:mainfrom
Conversation
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Fixes bot search behavior on the UI Bots page by making client-side filtering consistently case-insensitive (and normalized) across both the user’s search input and the bot fields being searched, addressing issue #26970.
Changes:
- Added a shared
filterBotsBySearchTermhelper to centralize bot search matching logic. - Updated
BotListV1to use the shared helper instead of inline filtering logic. - Added regression unit tests to cover display name/name matching and email-style identifier matching.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| openmetadata-ui/src/main/resources/ui/src/utils/BotsUtils.tsx | Introduces shared bot filtering helper with consistent normalization across searched fields. |
| openmetadata-ui/src/main/resources/ui/src/utils/BotsUtils.test.tsx | Adds unit tests validating case-insensitive and email-style bot search matching. |
| openmetadata-ui/src/main/resources/ui/src/components/Settings/Bot/BotListV1/BotListV1.component.tsx | Switches Bots page filtering to use the shared helper and removes now-unneeded inline normalization. |
|
@siddhiguptas we need to implement api search here and not client side search, please make changes accordingly |
mohityadav766
added
the
safe to test
Jest test CoverageUI tests summary
|
🟡 Playwright Results — all passed (13 flaky)✅ 3960 passed · ❌ 0 failed · 🟡 13 flaky · ⏭️ 86 skipped
🟡 13 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
…l lookup with robust partial email matching
…nd hardening glossary/tag and cleanup flows
…bot cleanup assertions in Playwright
|
hey @harsh-vador I tried to fix things but that one Playwright tests is still falling. I tired to look into the logs and the test is failing for the files we haven't touched. |
|
@siddhiguptas can you please update the PR description with screen recording? |
@harsh-vador updated |
| const enrichBotWithMatchedUser = useCallback((bot: Bot, botUser?: User) => { | ||
| if (!botUser) { | ||
| return bot; | ||
| } | ||
|
|
||
| return { | ||
| ...bot, | ||
| botUser: { | ||
| ...(bot.botUser ?? {}), | ||
| id: botUser.id, | ||
| name: botUser.name, | ||
| displayName: botUser.displayName, | ||
| fullyQualifiedName: botUser.fullyQualifiedName, | ||
| email: botUser.email, | ||
| } as Bot['botUser'], | ||
| }; | ||
| }, []); | ||
|
|
||
| const enrichBotsWithBotUsers = async (bots: Bot[]) => { | ||
| if (!bots.length) { | ||
| return bots; |
There was a problem hiding this comment.
seems a repetitive code here, please refactor
…licate bot user mapping logic
|
Hi @harsh-vador, I investigated the latest CI run and found two tracks: for the bot flow, the timeout appears tied to the new shared search helper predicate being too strict around search API matching, and for the rest, the failures look out-of-scope to this PR ( |
|
@siddhiguptas i still see |
|
Hi, @harsh-vador. I audited the failing Bots Playwright flow and fixed the flakiness in playwright/utils/bot.ts by removing strict waitForResponse dependency during bot search validation, since that wait was intermittently timing out while UI behavior remained correct; Bots.spec.ts was not changed. |
…guptas/OpenMetadata into fix/bots-page-name-search
|
@harsh-vador fixed everything |
| interface SearchInputOptions { | ||
| waitForSearchApi?: boolean; | ||
| searchApiPattern?: string; | ||
| } |
There was a problem hiding this comment.
please avoi declaring interface like this
| ) => { | ||
| await searchInput.clear(); | ||
| await searchInput.fill(value); | ||
| await expect(searchInput).toHaveValue(value); |
There was a problem hiding this comment.
integrate the search api get api here as well
| waitForSearchApi && searchTerm | ||
| ? page.waitForResponse( | ||
| (response) => | ||
| response.url().includes(searchApiPattern) && | ||
| response.request().method() === 'GET' && | ||
| response.url().includes(encodeURIComponent(searchTerm)) | ||
| ) | ||
| : undefined; |
There was a problem hiding this comment.
this does not need to be conditional
|
|
||
| await updateSearchInputAndWait(page, searchInput, searchTerm); | ||
|
|
||
| if (searchResponsePromise) { |
There was a problem hiding this comment.
please remove this if block
| const BOT_SEARCH_PAGE_SIZE = 100; | ||
| const BOT_SEARCH_CONCURRENCY = 10; | ||
| const MAX_BOT_SEARCH_PAGES = 5; |
There was a problem hiding this comment.
we already have this constants defined, take a look on the Users page implementation
| const enrichBotWithMatchedUser = useCallback((bot: Bot, botUser?: User) => { | ||
| if (!botUser) { | ||
| return bot; | ||
| } | ||
|
|
||
| return { | ||
| ...bot, | ||
| botUser: { | ||
| ...(bot.botUser ?? {}), | ||
| id: botUser.id, | ||
| name: botUser.name, | ||
| displayName: botUser.displayName, | ||
| fullyQualifiedName: botUser.fullyQualifiedName, | ||
| email: botUser.email, | ||
| } as Bot['botUser'], | ||
| }; | ||
| }, []); | ||
|
|
||
| const enrichBotsWithBotUsers = async (bots: Bot[]) => { | ||
| if (!bots.length) { | ||
| return bots; |
| const getSearchMatchedBotUsers = async ( | ||
| text: string, | ||
| queryFilter: Record<string, unknown> | ||
| ) => { | ||
| const usersByName = new Map<string, User>(); | ||
| let pageNumber = 1; | ||
| let totalMatches = 0; | ||
|
|
||
| while (pageNumber <= MAX_BOT_SEARCH_PAGES) { | ||
| const response = await searchQuery({ | ||
| query: text, | ||
| pageNumber, | ||
| pageSize: BOT_SEARCH_PAGE_SIZE, | ||
| includeDeleted: showDeleted, | ||
| queryFilter, | ||
| searchIndex: SearchIndex.USER, | ||
| trackTotalHits: true, | ||
| }); | ||
| const users = formatUsersResponse(response.hits.hits); |
There was a problem hiding this comment.
i see a many search & get bot user functions defined, we can simplify them , please do it
…liable name/email query behavior
| const getBotUserFromUser = (botUser: User): NonNullable<Bot['botUser']> => ({ | ||
| id: botUser.id, | ||
| name: botUser.name, | ||
| displayName: botUser.displayName, | ||
| fullyQualifiedName: botUser.fullyQualifiedName, | ||
| email: botUser.email, | ||
| }); |
There was a problem hiding this comment.
💡 Bug: getBotUserFromUser is not memoized, invalidating useCallback
getBotUserFromUser (line 108) is a plain function declared inside the component body, so it is recreated on every render. Listing it in the dependency array of enrichBotWithMatchedUser's useCallback (line 130) means the callback is regenerated every render, defeating the purpose of useCallback. Either wrap getBotUserFromUser in its own useCallback, or since it captures no reactive state, move it outside the component or inline it.
Suggested fix:
// Move getBotUserFromUser outside the component (it has no dependencies)
const getBotUserFromUser = (botUser: User): NonNullable<Bot['botUser']> => ({
id: botUser.id,
name: botUser.name,
displayName: botUser.displayName,
fullyQualifiedName: botUser.fullyQualifiedName,
email: botUser.email,
});
// Then remove it from the useCallback dependency array:
const enrichBotWithMatchedUser = useCallback(
(bot: Bot, botUser?: User) => { ... },
[]
);
Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion
| const searchResponsePromise = waitForSearchApi | ||
| ? page.waitForResponse( | ||
| (response) => | ||
| response.url().includes(searchApiPattern) && | ||
| response.request().method() === 'GET' && | ||
| response.url().includes(encodeURIComponent(searchTerm)) | ||
| ) |
There was a problem hiding this comment.
💡 Edge Case: searchFromSearchInput waits on API even for empty search term
The old code guarded waitForResponse with waitForSearchApi && searchTerm. The new code (line 228) only checks waitForSearchApi. When searchTerm is empty, encodeURIComponent('') is '', and url.includes('') is always true, so the promise will match the first GET to the search API pattern regardless of query content. This is unlikely to cause failures in current callers (all pass non-empty terms), but it weakens the assertion and could cause flaky matches if the helper is reused with an empty term in the future.
Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion
Code Review 👍 Approved with suggestions 11 resolved / 13 findingsRefactored the bot search logic to eliminate N+1 API calls, race conditions, and inefficient data retrieval. Memoize the 💡 Bug: getBotUserFromUser is not memoized, invalidating useCallback📄 openmetadata-ui/src/main/resources/ui/src/components/Settings/Bot/BotListV1/BotListV1.component.tsx:108-114 📄 openmetadata-ui/src/main/resources/ui/src/components/Settings/Bot/BotListV1/BotListV1.component.tsx:130
Suggested fix💡 Edge Case: searchFromSearchInput waits on API even for empty search term📄 openmetadata-ui/src/main/resources/ui/playwright/utils/common.ts:228-234 The old code guarded ✅ 11 resolved✅ Edge Case: lodash lowerCase strips special chars, broadening search
✅ Performance: N+1 API calls: getBotByName called individually per matched user
✅ Bug: Stale search results from race condition in handleSearch
✅ Quality: Duplicated bot-user enrichment logic in two functions
✅ Performance: getBotsByBotUserNames scans all bots instead of direct lookup
...and 6 more resolved from earlier reviews 🤖 Prompt for agentsOptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
❌ UI Checkstyle Failed❌ ESLint + Prettier + Organise Imports (src)One or more source files have linting or formatting issues. Affected files
Fix locally (fast — only checks files changed in this branch): make ui-checkstyle-changed |
|
Describe your changes:
Fixes #26970
This change replaces client-side bot filtering on the Bots page with API-driven search so bot name and bot email searches both work reliably.
I worked on this because bot search should be consistent and accurate for both bot names and email-like identifiers, and API search is the expected behavior for this page.
Changes made:
BotListV1and switched search flow to APIgetBotByNameinbotsAPI.tsand used it to resolve matched bot users into bot entitiessearchQuery(SearchIndex.USER+isBot:true)BotsUtils.tsxandBotsUtils.test.tsxplaywright/e2e/Pages/Bots.spec.tsplaywright/utils/bot.tsTesting:
git diff --checkyarn lint:base src/components/Settings/Bot/BotListV1/BotListV1.component.tsx src/rest/botsAPI.ts src/utils/BotsUtils.tsx src/utils/BotsUtils.test.tsxyarn lint:base playwright/e2e/Pages/Bots.spec.ts playwright/utils/bot.tsyarn pretty:base --check src/components/Settings/Bot/BotListV1/BotListV1.component.tsx src/rest/botsAPI.ts src/utils/BotsUtils.tsx src/utils/BotsUtils.test.tsx playwright/e2e/Pages/Bots.spec.ts playwright/utils/bot.tsyarn test src/components/Settings/Bot/BotListV1/BotListV1.component.test.tsx --watch=falseyarn test src/utils/BotsUtils.test.tsx --watch=falseNote:
make ui-checkstyle-src-changedcould not be completed in this environment due missingantlr4binary in PATH during UI postinstall/build-check.Type of change:
Checklist:
Fixes <issue-number>: <short explanation>Not applicable. This PR only updates UI/API search behavior and related tests; there is no schema or migration impact.
Screen Recording - https://drive.google.com/file/d/1HisqYIXrGDWtM9-nFI-urc2FXB2S58vk/view?usp=drive_link
Summary by Gitar
searchFromSearchInputandupdateSearchInputAndWaitincommon.tsto handle standardized search input with API response waiting.verifyBotSearchutility to validate bot searching by name and email inbot.ts.verifyBotSearchinto the mainBots.spec.tstest suite.This will update automatically on new commits.